fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies #4049

Vadman97 · 2023-08-09T23:51:13Z

Which problem is this PR solving?

@opentelemetry/sdk-node depends on @opentelemetry/exporter-jaeger despite never using it directly.
This increases the number of dependencies required to install @opentelemetry/sdk-node even if
@opentelemetry/exporter-jaeger is not used.

Fixes #3759

Short description of the changes

The change moves @opentelemetry/exporter-jaeger to the dev dependencies to resolve it in tests
but avoid requiring it for general usage of @opentelemetry/sdk-node. This is consistent with the
@opentelemetry/sdk-node documentation which states
that exporters must be explicit dependencies.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Locally building and testing the package after running npm install in opentelemetry-js/experimental/packages/opentelemetry-sdk-node.

Checklist:

Followed the style guidelines of this project
Unit tests have been added - N/A
Documentation has been updated - NA (already consistent)

linux-foundation-easycla · 2023-08-09T23:51:19Z

The committers listed above are authorized under a signed CLA.

✅ login: Vadman97 / name: Vadim Korolik (ae4acdc, 4c1dc82)

experimental/packages/opentelemetry-sdk-node/package.json

codecov · 2023-08-10T05:03:46Z

Codecov Report

Merging #4049 (09b7308) into main (4f28f90) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 09b7308 differs from pull request most recent head 4c1dc82. Consider uploading reports for the commit 4c1dc82 to get more accurate results

@@            Coverage Diff             @@
##             main    #4049      +/-   ##
==========================================
+ Coverage   92.56%   92.57%   +0.01%     
==========================================
  Files         292      292              
  Lines        8178     8178              
  Branches     1692     1692              
==========================================
+ Hits         7570     7571       +1     
+ Misses        608      607       -1

Flarna · 2023-08-10T05:48:34Z

As you can see in the test run a changelog entry is needed.
Not sure if this should be marked as breaking. At least a clears statement in the changelog should me made that an explicit dependency is needed now. Maybe even improve the corresponding section in readme to make this clearer.

Vadman97 · 2023-08-10T06:51:22Z

Thank you @Flarna! Will add the changelog entry shortly and will update the readme as well.

pichlermarc · 2023-08-10T08:52:44Z

Thank you for your contribution @Vadman97, removing this exporter has been long overdue 🙂

The exporter is lazy-required in sdk-node to allow sdk-node to work with bundling tools as the Jaeger exporter does not support them. We can remove it, but it will break users with the environment variable set.

opentelemetry-js/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts

Line 76 in d3436bf

const { JaegerExporter } = require('@opentelemetry/exporter-jaeger');

We'll also have to adapt the error message that the exporter has to be installed manually.

opentelemetry-js/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts

Lines 79 to 81 in d3436bf

    
           throw new Error( 
        
             `Could not instantiate JaegerExporter. This could be due to the JaegerExporter's lack of support for bundling. If possible, use @opentelemetry/exporter-trace-otlp-proto instead. Original Error: ${e}` 
        
           );

Previously we've gone the way of releasing a version that adds a postinstall script that alerts users that a feature will disappear in the next release (see #3833).

As @opentelemetry/sdk-node is widely used, I propose we do the same before merging this PR. It will delay this PR for a bit, but it will give users some advance notice that their telemetry will go missing when they update. 🤔

See also:

pichlermarc

Thank you for working on this; this looks good as-is 🙂

For lack of a better way to do it, I'm just using the "Changes Requested" state to block this until #4068 is merged and released (as mentioned in #4049 (comment)). Once we have it released, we can merge this PR 🙂

Vadman97 · 2023-08-22T21:15:36Z

Thanks for your help @pichlermarc , and sorry for the delay here. I'm updating the branch now to change TracerProviderWithEnvExporter.ts as you described.

Vadman97 · 2023-08-31T23:13:13Z

@pichlermarc - is this ready to merge now that #4068 is in?

pichlermarc · 2023-09-01T12:41:34Z

@pichlermarc - is this ready to merge now that #4068 is in?

Yes, but we still need to cut a release first to get #4068 out 🙂
I'm going to release 0.42.0 on Monday (trying not to release on Friday) - after that is done we can merge this in.

Sorry for the delay.

erikshestopal · 2023-09-05T17:41:25Z

@pichlermarc Seems like we can merge this PR since #4068 is out?

pichlermarc

ready to merge now that the release it out.

Adds README and CHANGELOG documentation of the change.

Vadman97 · 2023-09-19T15:46:12Z

hey @pichlermarc , sorry to pester here - is there any estimate as to when the next version will be released to npm so that we can update our dependency on @opentelemetry/sdk-node?

pichlermarc · 2023-09-20T08:46:47Z

The release with the warning message went out last week; I'd cut another release mid-next week.

Vadman97 · 2023-10-04T00:34:52Z

Hey @pichlermarc , any updates here on the next release being shipped?

pichlermarc · 2023-10-04T06:17:13Z

@Vadman97 release PR is open at #4183

## Summary References new opentelemetry dependencies with open-telemetry/opentelemetry-js#4049 merged Explicitly makes `@opentelemetry/exporter-jaeger` a dev dependency so that consumers do not reference it in their bundle. ## How did you test this change? Local e2e app using yalc published SDK. repro app is working correctly https://discord.com/channels/1026884757667188757/1138948448507404338 ![image](https://github.com/highlight/highlight/assets/1351531/31b87688-d2de-40dc-80cf-c7946b5a1eee) ## Are there any deployment considerations? Changeset included. ## Does this work require review from our design team? No

Vadman97 marked this pull request as ready for review August 10, 2023 00:59

Vadman97 requested a review from a team August 10, 2023 00:59

Vadman97 changed the title ~~move @opentelemetry/exporter-jaeger to dev dependencies~~ fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies Aug 10, 2023

Flarna reviewed Aug 10, 2023

View reviewed changes

experimental/packages/opentelemetry-sdk-node/package.json Outdated Show resolved Hide resolved

Flarna approved these changes Aug 10, 2023

View reviewed changes

Vadman97 force-pushed the main branch from 21f05bf to 94da919 Compare August 11, 2023 01:30

svetlanabrennan approved these changes Aug 11, 2023

View reviewed changes

pichlermarc mentioned this pull request Aug 16, 2023

chore(sdk-node): add notice to manually install '@opentelemetry/exporter-jaeger' #4068

Merged

2 tasks

pichlermarc requested changes Aug 16, 2023

View reviewed changes

pichlermarc added the Dependency on other PR This PR can't be merged because it has dependency on other PRs label Aug 16, 2023

Vadman97 force-pushed the main branch from bc8cf07 to fe05e70 Compare August 22, 2023 21:26

Vadman97 force-pushed the main branch from fe05e70 to 499a910 Compare August 31, 2023 06:20

Vadman97 requested a review from pichlermarc August 31, 2023 06:20

Vadman97 force-pushed the main branch from 499a910 to 9ec3fc4 Compare August 31, 2023 23:12

Vadman97 force-pushed the main branch 2 times, most recently from f43904f to 2c69fb1 Compare September 11, 2023 22:16

pichlermarc approved these changes Sep 12, 2023

View reviewed changes

Vadman97 closed this Sep 12, 2023

Vadman97 force-pushed the main branch from 2c69fb1 to 4f28f90 Compare September 12, 2023 20:10

Vadman97 added 2 commits September 12, 2023 13:10

move @opentelemetry/exporter-jaeger to dev dependencies

ae4acdc

fixup! move @opentelemetry/exporter-jaeger to dev dependencies

4c1dc82

Adds README and CHANGELOG documentation of the change.

Vadman97 reopened this Sep 12, 2023

pichlermarc merged commit 5615b1c into open-telemetry:main Sep 13, 2023

Flarna mentioned this pull request Sep 15, 2023

Remove dependency to all exporters in SDK #4150

Open

Vadman97 mentioned this pull request Oct 4, 2023

release new sdk versions without jaeger dependency highlight/highlight#6797

Closed

Vadman97 mentioned this pull request Oct 10, 2023

release new sdk versions without jaeger dependency highlight/highlight#6838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies #4049

fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies #4049

Vadman97 commented Aug 9, 2023 •

edited by pichlermarc

Loading

linux-foundation-easycla bot commented Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading

Flarna commented Aug 10, 2023

Vadman97 commented Aug 10, 2023

pichlermarc commented Aug 10, 2023

pichlermarc left a comment

Vadman97 commented Aug 22, 2023

Vadman97 commented Aug 31, 2023

pichlermarc commented Sep 1, 2023

erikshestopal commented Sep 5, 2023

pichlermarc left a comment

Vadman97 commented Sep 19, 2023

pichlermarc commented Sep 20, 2023

Vadman97 commented Oct 4, 2023

pichlermarc commented Oct 4, 2023

fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies #4049

fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies #4049

Conversation

Vadman97 commented Aug 9, 2023 • edited by pichlermarc Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Aug 9, 2023 • edited Loading

codecov bot commented Aug 10, 2023 • edited Loading

Codecov Report

Flarna commented Aug 10, 2023

Vadman97 commented Aug 10, 2023

pichlermarc commented Aug 10, 2023

pichlermarc left a comment

Choose a reason for hiding this comment

Vadman97 commented Aug 22, 2023

Vadman97 commented Aug 31, 2023

pichlermarc commented Sep 1, 2023

erikshestopal commented Sep 5, 2023

pichlermarc left a comment

Choose a reason for hiding this comment

Vadman97 commented Sep 19, 2023

pichlermarc commented Sep 20, 2023

Vadman97 commented Oct 4, 2023

pichlermarc commented Oct 4, 2023

Vadman97 commented Aug 9, 2023 •

edited by pichlermarc

Loading

linux-foundation-easycla bot commented Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading